chore: Add mutex wrapper from clio#6447
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new mutex+data wrapper (ported from clio) intended to bundle protected data with its mutex and provide an RAII lock object that grants scoped access to the data.
Changes:
- Add
xrpl::Mutex<T, MutexType>container that ownsTand a mutex. - Add
xrpl::Lock<T, LockType, MutexType>to hold the lock and provide*,->, andget()accessors.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6447 +/- ##
=========================================
- Coverage 79.8% 79.8% -0.0%
=========================================
Files 858 859 +1
Lines 67761 67780 +19
Branches 7547 7552 +5
=========================================
+ Hits 54084 54097 +13
- Misses 13677 13683 +6
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
vlntb
left a comment
vlntb
left a comment
There was a problem hiding this comment.
LGTM. Let's resolve the question about the unit test, and I'm happy to approve.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kuznetsss
added
the
Ready to merge
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ximinez
left a comment
ximinez
left a comment
There was a problem hiding this comment.
This is not a "chore". It is a new feature. Just because it's not user-facing doesn't mean it's not new.
| std::unique_lock<std::mutex>& ul = lock; | ||
| ul.unlock(); | ||
| ul.lock(); |
There was a problem hiding this comment.
Does this leave the data vulnerable to modification outside of lock?
| auto lock = const_ref.lock(); | ||
| EXPECT_EQ(lock->size(), 6); | ||
| EXPECT_EQ(lock->at(5), 6); | ||
| // lock->push_back(7); // Should not compile (const) |
There was a problem hiding this comment.
static_assert(std::is_const_v<decltype(lock)>); or something like that for all these tests where you're expecting a const lock, and have code commented out that "should not compile".
High Level Overview of Change
This PR adds a mutex wrapper copied from clio. This wrapper makes a mutex attached to the data it protects which improves safety and readability.
Context of Change
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)